Fix uint overflow bugs in std::{at_vec, vec, str} #9108

bluss · 2013-09-11T00:58:34Z

Fix uint overflow bugs in std::{at_vec, vec, str}

Fix issue #8742, which summarized is: unsafe code in vec and str did assume
that a reservation for X + Y elements always succeeded, and didn't overflow.

Introduce the method Vec::reserve_additional(n) to make it easy to check for
overflow in Vec::push and Vec::push_all.

In std::str, simplify and remove a lot of the unsafe code and use push_str
instead. With improvements to .push_str and the new function
vec::bytes::push_bytes, it looks like this change has either no or positive
impact on performance.

I believe there are many places still where v.reserve(A + B) still can overflow.
This by itself is not an issue unless followed by (unsafe) code that steps aside
boundary checks.

emberian · 2013-09-11T01:18:49Z

LGTM. I'm ok with the perf hit.

bluss · 2013-09-11T19:46:23Z

Most of the removal of unsafe code and simplification is because I don't want to check for overflow everywhere. It's messy, and we can let boundary checks or .reserve() do the work most of the time. However we may need a better plan. What do we really want to do for the v.reserve(X + Y) case. Ignore the overflow in addition and handle it in bounds checks? Change the "size_t" type to something that always uses checked addition?

erickt · 2013-09-11T22:25:35Z

Bounds checking sounds like a good candidate for a __builtin_expect-style function that hints to the branch predictor to assume we'll almost always not overflow when resizing. Do we have anything like that in the compiler, and if not, how hard would it be to add it?

emberian · 2013-09-11T22:28:53Z

More importantly: how much impact would it actually have?

On Wed, Sep 11, 2013 at 6:25 PM, Erick Tryzelaar
notifications@github.comwrote:

Bounds checking sounds like a good candidate for a __builtin_expecthttp://llvm.org/docs/BranchWeightMetadata.html-style
function that hints to the branch predictor to assume we'll almost always
not overflow when resizing. Do we have anything like that in the compiler,
and if not, how hard would it be to add it?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/9108#issuecomment-24281515
.

emberian · 2013-09-11T22:53:05Z

+1 to branch prediction hints; conversation that convinced me at https://botbot.me/mozilla/rust/msg/5945880/

brson · 2013-09-12T21:53:31Z

I'm very skeptical of branch prediction hints generally. It's not uncommon to annotate them incorrectly, or for them to be useless.

thestinger · 2013-09-12T22:12:31Z

I have a feeling that LLVM's branch prediction hints don't do much when you aren't optimizing for size.

bluss · 2013-09-13T01:13:57Z

it's not an important part. More important would be to reduce the number of places we need to check for overflow

Like next_power_of_two, but returns None on overflow.

bluss · 2013-09-16T16:17:20Z

Rebased to avoid conflicts (new name of Option::unwrap_or).

I changed .reserve_additional to check the vec capacity before doing anything; I think this is an improvement that puts the failure case (in this case overflow) inside a branch that is only taken seldomly.

I changed vec::bytes::push_bytes to not use .mut_slice() but to use ptr::copy_memory directly. As the commit log says, It's good to avoid overhead and use a tad more unsafe code since much of str will depend on this function.

With these changes, it looks like the StrVector method .connect() is faster with its safe implementation on top of the change series, than its unsafe implementation before.

I'm not sure if I'm benchmarking this correctly. It could be that .connect() just optimizes to the point of invalidating the benchmark with the changes, but that's a win in itself..

with local rustc --opt-level=3 -Z no-monomorphic-collapse --test std.rs

before
test str::bench::bench_connect ... bench: 333 ns/iter (+/- 2)
test str::bench::bench_push_str ... bench: 163 ns/iter (+/- 4)

after
test str::bench::bench_connect ... bench: 178 ns/iter (+/- 3)
test str::bench::bench_push_str ... bench: 162 ns/iter (+/- 2)

I'm not sure if the benchmarks should stay in the PR, but apart from that I think this change is ready for review & merge.

bluss · 2013-09-16T16:20:35Z

Regarding branch predicition, since the overflow check is now inside a rarely taken if, it's less of an issue. I'd guess that using CheckedAdd we give enough information already to llvm to adjust branch prediction.

bluss · 2013-09-16T16:31:51Z

Updated the PR text (because this information will show in the merge message, so I want it to be correct)

bluss · 2013-09-16T16:55:19Z

Now the stage2 bench is done for before and after:

before:
test str::bench::bench_connect ... bench: 302 ns/iter (+/- 7)
test str::bench::bench_push_str ... bench: 140 ns/iter (+/- 3)

after
test str::bench::bench_connect ... bench: 225 ns/iter (+/- 24)
test str::bench::bench_push_str ... bench: 139 ns/iter (+/- 2)

Issue #8742 Add the method `.reserve_additional(n: uint)`: Check for overflow in self.len() + n, and reserve that many elements (rounded up to next power of two). Does nothing if self.len() + n < self.capacity() already.

`push_bytes` is implemented with `ptr::copy_memory` here since this function is intended to be used to implement `.push_str()` for str, so we want to avoid the overhead.

alexcrichton · 2013-09-17T00:15:58Z

src/libstd/str.rs

+    /// Access the str in its vector representation.
+    /// The caller must preserve the valid UTF-8 property when modifying.
+    #[inline]
+    pub unsafe fn as_owned_vec<U>(s: &mut ~str, f: &fn(&mut ~[u8]) -> U) -> U {


Could this be expressed as fn as_owned_vec<'a>(s: &'a mut ~str) -> &'a mut ~[u8] ?

alexcrichton · 2013-09-17T00:20:24Z

This looks good to me! Sad to see a positive diff stat, but I guess that's what happens when you write tests :)

r+ with my comment

See issue #8742

@1

Easy to reproduce: let mut v = ~[@1]; v.resize(-1); // success a.k.a silent failure v.push(@2); // segfault

bluss · 2013-09-17T00:50:45Z

Incorporated your suggestion, also found one place where as_owned_vec could be used (as_mut_buf), so those are the changes squashed into the std::str: Fix overflow.. commit.

…hton Fix uint overflow bugs in std::{at_vec, vec, str} Closes #8742 Fix issue #8742, which summarized is: unsafe code in vec and str did assume that a reservation for `X + Y` elements always succeeded, and didn't overflow. Introduce the method `Vec::reserve_additional(n)` to make it easy to check for overflow in `Vec::push` and `Vec::push_all`. In std::str, simplify and remove a lot of the unsafe code and use `push_str` instead. With improvements to `.push_str` and the new function `vec::bytes::push_bytes`, it looks like this change has either no or positive impact on performance. I believe there are many places still where `v.reserve(A + B)` still can overflow. This by itself is not an issue unless followed by (unsafe) code that steps aside boundary checks.

…xFrednet Remove trailing spaces Closes rust-lang#9108 changelog: remove trailing spaces, which are not allowed by the JSON standard.

std::num: Add uint::next_power_of_two_opt

2017cc3

Like next_power_of_two, but returns None on overflow.

blake2-ppc added 3 commits September 16, 2013 19:13

std::vec: Fix hazards with uint overflows in unsafe code

8d488f3

Issue #8742 Add the method `.reserve_additional(n: uint)`: Check for overflow in self.len() + n, and reserve that many elements (rounded up to next power of two). Does nothing if self.len() + n < self.capacity() already.

std::vec: Add function vec::bytes::push_bytes

8fce135

`push_bytes` is implemented with `ptr::copy_memory` here since this function is intended to be used to implement `.push_str()` for str, so we want to avoid the overhead.

std::str: Add bench tests for StrVector::connect() and for str::push_str

e34e203

alexcrichton reviewed Sep 17, 2013
View reviewed changes

blake2-ppc added 3 commits September 17, 2013 02:47

std::str: Fix overflow problems in unsafe code

90dc951

See issue #8742

rt::io: Use vec::reserve_additional

6e538ed

std::at_vec: Fix segfault on overflow when resizing ~[@t]

e211888

Easy to reproduce: let mut v = ~[@1]; v.resize(-1); // success a.k.a silent failure v.push(@2); // segfault

bors closed this Sep 17, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix uint overflow bugs in std::{at_vec, vec, str} #9108

Fix uint overflow bugs in std::{at_vec, vec, str} #9108

bluss commented Sep 11, 2013

emberian commented Sep 11, 2013

bluss commented Sep 11, 2013

erickt commented Sep 11, 2013

emberian commented Sep 11, 2013

emberian commented Sep 11, 2013

brson commented Sep 12, 2013

thestinger commented Sep 12, 2013

bluss commented Sep 13, 2013

bluss commented Sep 16, 2013

bluss commented Sep 16, 2013

bluss commented Sep 16, 2013

bluss commented Sep 16, 2013

alexcrichton Sep 17, 2013

alexcrichton commented Sep 17, 2013

bluss commented Sep 17, 2013

Fix uint overflow bugs in std::{at_vec, vec, str} #9108

Fix uint overflow bugs in std::{at_vec, vec, str} #9108

Conversation

bluss commented Sep 11, 2013

emberian commented Sep 11, 2013

bluss commented Sep 11, 2013

erickt commented Sep 11, 2013

emberian commented Sep 11, 2013

emberian commented Sep 11, 2013

brson commented Sep 12, 2013

thestinger commented Sep 12, 2013

bluss commented Sep 13, 2013

bluss commented Sep 16, 2013

bluss commented Sep 16, 2013

bluss commented Sep 16, 2013

bluss commented Sep 16, 2013

alexcrichton Sep 17, 2013

Choose a reason for hiding this comment

alexcrichton commented Sep 17, 2013

bluss commented Sep 17, 2013